The re-allocated space might not be used if no further insertion. Shall we do growAndRehash at the beginning of append when numKeys == growthThreshold && !isDefined?

Unfortunately, we can't grow in the beginning, otherwise the pos will be wrong.

OK. LGTM.

Unrelated, but this OutOfMemoryError will not be useful - atleast not in yarn mode.
It will simply cause the jvm to exit.

This does not look correct as per documentation of MAX_CAPACITY.
Actual number of keys == MAX_CAPACITY (so that total number of entries in longArray is MAX_CAPACITY * 2)

We grow the array when numKeys >= growthThreshold and growthThreshold = capacity * 0.5. But we actually allocate capacity * 2 spaces for the array.

So actually numKeys < growthThreshold = capacity * 0.5 < array length = capacity * 2 should hold true.

Because numKeys < growthThreshold is always true, if numKeys == MAX_CAPACITY, the capacity would be MAX_CAPACITY * 2 at least and the length of array will be more than MAX_CAPACITY * 4.

But in allocate, there is an assert of capacity <= MAX_CAPACITY. Looks like those condition are inconsistent.

Also, we need to move the appropriate validation check into growAndRehash() and not here.

There are two reason it will fail to grow: 1) current capacity (longArray.size() / 2) reach MAX_CAPACITY 2) can't allocate a array (OOM).

So, I think the checking here is correct.

@davies You are right that the check for longArray.size() / 2 == MAX_CAPACITY is the upper bound beyond which we cant grow. It is simply confusing it do it outside growAndRehash - which is what threw me off.
Please move the check into growAndRehash() and have it return true in case it could successfully grow the map.

@mridulm Do that means we should also rename the growAndRehash to tryGrowAndRehash? I think those are not necessary.

The invariant in question is for growAndRehash() - not append, and as such should live there.
Code evolution causing grow to be invoked from elsewhere will require duplication of the invariant everywhere.

Btw, this is in line with all other data structures spark (and other frameworks) have.

This longArray.size() < MAX_CAPACITY should be wrong condition.

longArray.size() is the next capacity for current grow strategy, it should be longArray.size() <= MAX_CAPACITY

Is MAX_CAPACITY still the maximum number of keys as per documentation of it? If we can have longArray.size() / 2 == MAX_CAPACITY at most for the capacity, the actually numKeys should be MAX_CAPACITY / 2, because we need two long array entries per key, right?

@davies is correct; but it is a slightly unintuitive way to write the condition.

val currentSize = longArray.size()
val newSize = currentSize * 2
val currentKeysLen = currentSize / 2
val newKeysLen = currentKeysLen * 2

if (newKeysLen > MAX_CAPACITY) then fail.
that is  if (currentKeysLen == MAX_CAPACITY) then fail // Since we allow only power of 2's for all these values.
that is  if (longArray.size() / 2 == MAX_CAPACITY)

Particularly given its location (in append as opposed to grow), it serves to be a bit more confusing that expected.

If the currentKeysLen above is the number of keys, it never equals to currentSize / 2. currentSize / 2 is actually the capacity we want to allocate (but actually we allocate double of it for the array).

Once the number of keys reaches growthThreshold (i.e., capacity * 0.5), we go to grow the array or fail the append. So the number of keys is always less than or equal to capacity * 0.5 which is currentSize * 0.5 * 0.5.

To clarify, the length's @davies and I mentioned are not actual number of keys in the map, but maximum number of keys possible in the map.

Ah, I see. It makes sense.

I think we need to grow the array only if isDefined == false.

This change makes sense to me because growthThreshold's Scaladoc says "The map will be expanded once the number of keys exceeds this threshold" and here we're considering the impact of adding an additional key (so this could have also been written as (numKeys + 1) > growthThreshold).